Skip to content

Fix link reach/role and add ancestors link access info #846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

sampaccoud
Copy link
Member

@sampaccoud sampaccoud commented Apr 6, 2025

Purpose

We are returning too many select options for the link access configuration.

Proposal

  • when the "restricted" reach is an option (key present in the returned dictionary), the possible values for link roles are now always None to make it clearer that they don't matter and no select box should be shown for roles.
  • Never propose "restricted" as option for link reach when the ancestors already offer a public access. Indeed, restricted/editor was shown when the ancestors had public/read access. The logic was to propose editor role on more restricted reaches... but this does not make sense for restricted since the role does is not taken into account for this reach. Roles are set by each access line assign to users/teams.
  • Never propose "authenticated" as option for link reach when the ancestors already offer a public access whatever the role. The logic was to propose editor role for authenticated user when the public access from ancestors was only readonly... but this was juged too much complexity for not enough value created. It can still be brought back later.
  • add ancestors links definitions to document abilities : the frontend needs to display inherited link accesses when it displays possible selection options. We need to return this information to the client.

We also realized that we needed to display specific access rights inherited from ancestors. This PR adds inherited accesses to the list of accesses returned for a document on /api/v1.0/documents/{uuid}/accesses/

@sampaccoud sampaccoud requested review from AntoLC and lunika April 6, 2025 19:29
@sampaccoud sampaccoud self-assigned this Apr 6, 2025
@sampaccoud sampaccoud added bug Something isn't working python Pull requests that update Python code backend refacto labels Apr 6, 2025
@sampaccoud sampaccoud force-pushed the fix-link_reach_role_and_add_ancestors_info branch 2 times, most recently from 28dac80 to d85d20f Compare April 12, 2025 16:44
@AntoLC AntoLC requested review from PanchoutNathan and removed request for AntoLC April 22, 2025 13:02
@AntoLC
Copy link
Collaborator

AntoLC commented Apr 22, 2025

@PanchoutNathan The request types documents/[DOC_ID]/accesses/ changed, it is not a pagination anymore.
See: https://github.com/suitenumerique/docs/blob/main/src/frontend/apps/impress/src/features/docs/doc-share/api/useDocAccesses.tsx

@sampaccoud sampaccoud force-pushed the fix-link_reach_role_and_add_ancestors_info branch from d85d20f to a8fda8f Compare April 28, 2025 06:05
We were returning too many select options for the restricted link reach:
- when the "restricted" reach is an option (key present in the returned
  dictionary), the possible values for link roles are now always None to
  make it clearer that they don't matter and no select box should be
  shown for roles.
- Never propose "restricted" as option for link reach when the ancestors
  already offer a public access. Indeed, restricted/editor was shown when
  the ancestors had public/read access. The logic was to propose editor
  role on more restricted reaches... but this does not make sense for
  restricted since the role does is not taken into account for this reach.
  Roles are set by each access line assign to users/teams.
The frontend needs to display inherited link accesses when it displays
possible selection options. We need to return this information to the
client.
If anonymous users have reader access on a parent, we were considering
that an edge use case was interesting: allowing an authenticated user
to still be editor on the child.

Although this use case could be interesting, we consider, as a first
approach, that the value it carries is not big enough to justify the
complexity for the user to understand this complex access right heritage.
The document viewset was overriding the get_queryset method from its
own mixin. This was a sign that the mixin was not optimal anymore.
In the next commit I will need to complexify it further so it's time
to refactor the mixin.
The methods to annotate a document queryset were factorized on the
viewset but the correct place is the custom queryset itself now that
we have one.
The document accesses a user have on a document's ancestors also apply
to this document. The frontend needs to list them as "inherited" so we
need to add them to the list.
Adding a "document_id" field on the output will allow the frontend to
differentiate between inherited and direct accesses on a document.
We are going to need to compare choices to materialize the fact that
choices are ordered. For example an admin role is higer than an
editor role but lower than an owner role.

We will need this to compute the reach and role resulting from all
the document accesses (resp. link accesses) assigned on a document's
ancestors.
This will allow us to simplify the get_abilities method. It is also
more efficient because we have computed this definitions dict and
the the get_select_options method was doing the conversion again.
@sampaccoud sampaccoud force-pushed the fix-link_reach_role_and_add_ancestors_info branch from a8fda8f to dda402f Compare April 28, 2025 06:21
sampaccoud added 6 commits May 2, 2025 09:21
The user account created to query the API had a random email that
could randomly interfere with our search results.
We were returning the list of roles a user has on a document (direct
and inherited). Now that we introduced priority on roles, we are able
to determine what is the max role and return only this one.

This commit also changes the role that is returned for the restricted
reach: we now return None because the role is not relevant in this
case.
On a document, we need to display the status of the link (reach and
role) as inherited from its ancestors.
On a document, we need to display the status of the link (reach and
role) taking into account the ancestors link reach/role as well as
the current document.
The latest refactoring in a445278 kept some factorizations that are
not legit anymore after the refactoring.

It is also cleaner to not make serializer choice in the list view if
the reason for this choice is related to something else b/c other
views would then use the wrong serializer and that would be a
security leak.

This commit also fixes a bug in the access rights inheritance: if a
user is allowed to see accesses on a document, he should see all
acesses related to ancestors, even the ancestors that he can not
read. This is because the access that was granted on all ancestors
also apply on the current document... so it must be displayed.

Lastly, we optimize database queries because the number of accesses
we fetch is going up with multi-pages and we were generating a lot
of useless queries.
This field is set only on the list view when all accesses for a given
document and all its ancestors are listed. It gives the highest role
among all accesses related to each document.
@sampaccoud sampaccoud force-pushed the fix-link_reach_role_and_add_ancestors_info branch from 990fee6 to a51e61c Compare May 2, 2025 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend bug Something isn't working python Pull requests that update Python code refacto
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants